Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify a few functions in rustc_data_structures #52697

Merged
merged 1 commit into from
Jul 30, 2018

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Jul 25, 2018

  • drop try!() where it's superfluous
  • change try!() to ?
  • squash a push with push_str
  • refactor a push loop into an iterator

@rust-highfive
Copy link
Collaborator

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 25, 2018
@@ -209,11 +209,7 @@ impl<A> Decodable for SmallVec<A>
A::Element: Decodable {
fn decode<D: Decoder>(d: &mut D) -> Result<SmallVec<A>, D::Error> {
d.read_seq(|d, len| {
let mut vec = SmallVec::with_capacity(len);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hunch that this was deliberately done for optimization

cc @nnethercote

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a good sense of how collect() works. Will it use size_hint from the range to pre-allocate an appropriate capacity?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, yes.

@kennytm kennytm closed this Jul 25, 2018
@kennytm kennytm reopened this Jul 25, 2018
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 29, 2018

📌 Commit 86d0e9e has been approved by Mark-Simulacrum

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 29, 2018
@bors
Copy link
Contributor

bors commented Jul 30, 2018

⌛ Testing commit 86d0e9e with merge 54628c8...

bors added a commit that referenced this pull request Jul 30, 2018
Simplify a few functions in rustc_data_structures

- drop `try!()` where it's superfluous
- change `try!()` to `?`
- squash a `push` with `push_str`
- refactor a push loop into an iterator
@bors
Copy link
Contributor

bors commented Jul 30, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 54628c8 to master...

@bors bors merged commit 86d0e9e into rust-lang:master Jul 30, 2018
@ljedrz ljedrz deleted the misc_data_structures branch July 30, 2018 14:21
cramertj added a commit to cramertj/rust that referenced this pull request Aug 2, 2018
Calculate capacity when collecting into Option and Result

I was browsing the [perf page](http://perf.rust-lang.org) to see the impact of my recent changes (e.g. rust-lang#52697) and I was surprised that some of the results were not as awesome as I expected. I dug some more and found an issue that is the probable culprit: [Collecting into a Result<Vec<_>> doesn't reserve the capacity in advance](rust-lang#48994).

Collecting into `Option` or `Result` might result in an empty collection, but there is no reason why we shouldn't provide a non-zero lower bound when we know the `Iterator` we are collecting from doesn't contain any `None` or `Err`.

We know this, because the `Adapter` iterator used in the `FromIterator` implementations for `Option` and `Result` registers if any `None` or `Err` are present in the `Iterator` in question; we can use this information and return a more accurate lower bound in case we know it won't be equal to zero.

I [have benchmarked](https://gist.github.com/ljedrz/c2fcc19f6260976ae7a46ae47aa71fb5) collecting into `Option` and `Result` using the current implementation and one with the proposed changes; I have also benchmarked a push loop with a known capacity as a reference that should be slower than using `FromIterator` (i.e. `collect()`). The results are quite promising:
```
test bench_collect_to_option_new ... bench:         246 ns/iter (+/- 23)
test bench_collect_to_option_old ... bench:         954 ns/iter (+/- 54)
test bench_collect_to_result_new ... bench:         250 ns/iter (+/- 25)
test bench_collect_to_result_old ... bench:         939 ns/iter (+/- 104)
test bench_push_loop_to_option   ... bench:         294 ns/iter (+/- 21)
test bench_push_loop_to_result   ... bench:         303 ns/iter (+/- 29)
```
Fixes rust-lang#48994.
kennytm added a commit to kennytm/rust that referenced this pull request Aug 10, 2018
Don't collect() when size_hint is useless

This adjusts PRs rust-lang#52738 and rust-lang#52697 by falling back to calculating capacity and extending or pushing in a loop where `collect()` can't be trusted to calculate the right capacity.

It is a performance win.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 12, 2018
Don't collect() when size_hint is useless

This adjusts PRs rust-lang#52738 and rust-lang#52697 by falling back to calculating capacity and extending or pushing in a loop where `collect()` can't be trusted to calculate the right capacity.

It is a performance win.
bors added a commit that referenced this pull request Feb 1, 2019
[WIP] Calculate capacity when collecting into Option and Result

I was browsing the [perf page](http://perf.rust-lang.org) to see the impact of my recent changes (e.g. #52697) and I was surprised that some of the results were not as awesome as I expected. I dug some more and found an issue that is the probable culprit: [Collecting into a Result<Vec<_>> doesn't reserve the capacity in advance](#48994).

Collecting into `Option` or `Result` might result in an empty collection, but there is no reason why we shouldn't provide a non-zero lower bound when we know the `Iterator` we are collecting from doesn't contain any `None` or `Err`.

We know this, because the `Adapter` iterator used in the `FromIterator` implementations for `Option` and `Result` registers if any `None` or `Err` are present in the `Iterator` in question; we can use this information and return a more accurate lower bound in case we know it won't be equal to zero.

I [have benchmarked](https://gist.github.com/ljedrz/c2fcc19f6260976ae7a46ae47aa71fb5) collecting into `Option` and `Result` using the current implementation and one with the proposed changes; I have also benchmarked a push loop with a known capacity as a reference that should be slower than using `FromIterator` (i.e. `collect()`). The results are quite promising:
```
test bench_collect_to_option_new ... bench:         246 ns/iter (+/- 23)
test bench_collect_to_option_old ... bench:         954 ns/iter (+/- 54)
test bench_collect_to_result_new ... bench:         250 ns/iter (+/- 25)
test bench_collect_to_result_old ... bench:         939 ns/iter (+/- 104)
test bench_push_loop_to_option   ... bench:         294 ns/iter (+/- 21)
test bench_push_loop_to_result   ... bench:         303 ns/iter (+/- 29)
```
Fixes #48994.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants